Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add update_monitors.sh script and readme for datadog migration #13

Merged
merged 5 commits into from
Apr 12, 2024

Conversation

AlexisJasso
Copy link
Contributor

https://firehydrant.atlassian.net/browse/PDE-1416

This is the script described in the above ticket for creating new datadog monitors with a new notification type based on existing monitors with a given existing notification type (or, optionally, modification of existing monitors). The readme should represent adequate documentation of the script and if not it should be updated.

Testing can be performed using the values in the secure note "Datadog Migration Script test params" in Engineering Secrets if desired.

@AlexisJasso AlexisJasso requested review from wilsonehusin and a team April 11, 2024 18:49
Copy link
Member

@wilsonehusin wilsonehusin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some comments — if we can do something like "about to run curl -X PUT ..., ok to go?" kind of prompt, I think it'd be superb!

@AlexisJasso
Copy link
Contributor Author

Made some comments — if we can do something like "about to run curl -X PUT ..., ok to go?" kind of prompt, I think it'd be superb!

Do we want to prompt for each one? We can do that, but I'm imagining that the desire for a script is to update dozens of these and that seems tedious.

@wilsonehusin
Copy link
Member

Made some comments — if we can do something like "about to run curl -X PUT ..., ok to go?" kind of prompt, I think it'd be superb!

Do we want to prompt for each one? We can do that, but I'm imagining that the desire for a script is to update dozens of these and that seems tedious.

we can print all of them ahead of time for one confirmation i think, but otherwise it's not really necessary.

Copy link
Member

@wilsonehusin wilsonehusin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with 1 non-blocking comment!

Comment on lines 59 to 65
echo "Continue?"
select yn in "Yes" "No"; do
case $yn in
Yes ) break;;
No ) exit;;
esac
done
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the default case for this? do we need * ) exit 1;;?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good call. Will add.

@AlexisJasso AlexisJasso merged commit 493db4c into main Apr 12, 2024
4 checks passed
@AlexisJasso AlexisJasso deleted the add_dd_migration_tools branch April 12, 2024 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants